-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: App builder bin 5.0 #8147
feat: App builder bin 5.0 #8147
Conversation
🦋 Changeset detectedLatest commit: 7f73cd2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for car-park-attendant-cleat-11576 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@mmaietta Please help review when you have time. |
if (pnpmIndex >= 0 && parts.length > pnpmIndex + 2) { | ||
// Remove '.pnpm' and the next two folders from the parts array | ||
parts.splice(pnpmIndex, 3) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we able to make this guarantee that the two nested folders within .pnpm
folder are okay to be removed? Are they unnecessary symlinks or something?
I have a question about conflict versions 2. The expected result is
I think the expected result is wrong, the right expected result is following:
@mmaietta What do you think? When dealing with conflict versions before, did the electron-buidler only keep one version? |
It looks like we're copying the same assets twice now, but I find it odd that the |
@@ -22,7 +22,7 @@ Object { | |||
"foo": Object { | |||
"files": Object { | |||
"package.json": Object { | |||
"offset": "0", | |||
"offset": "4310", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an odd change. What's at offset 0
now?
const destinationParts: string[] = destination.split(path.sep) | ||
|
||
const nodeModulesIndicesFilePath: number[] = filePathParts.reduce((acc: number[], part: string, index: number) => { | ||
if (part === "node_modules") acc.push(index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Please move this to new line
if (part === "node_modules") {
acc.push(index)
}
Same comment applies to line 41
You might have misunderstood. My example above was just that, directly copying data from MS modules. The approach of electron-builder is to copy two different versions of MS to the same directory, with the last copied version overriding the previous ones. I think there might be some issues with this approach, but it seems like no one has ever reported a problem. |
Ohh, interesting! I was curious as to how electron-builder has handled conflicting versions. Agreed, no one has ever reported a problem over the few years I've managed this project, so we may be able to proceed with your suggested/expected result as-is as it seems like it'd be the correct approach to me. |
Under the current implementation, this is not achievable. I have made modifications to the app-builder's implementation to achieve this. Additionally, after making these changes, we no longer need to handle pnmp separately. Taking test-app-yarn-workspace-version-conflict as an example, the old app-builder output format was like this: [
{
"dir": "~\\app-builder\\pkg\\node-modules\\yarn-demo\\node_modules",
"deps": [
{
"name": "ms",
"version": "2.0.0"
}
]
},
{
"dir": "~\\app-builder\\pkg\\node-modules\\yarn-demo\\packages\\test-app\\node_modules",
"deps": [
{
"name": "foo",
"version": "1.0.0"
},
{
"name": "ms",
"version": "2.1.1"
}
]
}
] After the changes, the output format is like this: [
{
"name": "foo",
"version": "1.0.0",
"dir": "~\\app-builder\\pkg\\node-modules\\yarn-demo\\packages\\foo",
"conflictDependency": [
{
"name": "ms",
"version": "2.0.0",
"dir": "~\\app-builder\\pkg\\node-modules\\yarn-demo\\node_modules\\ms"
}
]
},
{
"name": "ms",
"version": "2.1.1",
"dir": "~\\app-builder\\pkg\\node-modules\\yarn-demo\\packages\\test-app\\node_modules\\ms"
}
] Another exmaple - pnpm-demo: [
{
"dir": "~\\app-builder\\pkg\\node-modules\\pnpm-demo\\node_modules",
"deps": [
{
"name": "react",
"version": "18.2.0"
}
]
},
{
"dir": "~\\app-builder\\pkg\\node-modules\\pnpm-demo\\node_modules\\.pnpm\\loose-envify@1.4.0\\node_modules",
"deps": [
{
"name": "js-tokens",
"version": "4.0.0"
}
]
},
{
"dir": "~\\app-builder\\pkg\\node-modules\\pnpm-demo\\node_modules\\.pnpm\\react@18.2.0\\node_modules\\react\\node_modules",
"deps": [
{
"name": "loose-envify",
"version": "1.4.0"
}
]
}
] new format: [
{
"name": "js-tokens",
"version": "4.0.0",
"dir": "~\\app-builder\\pkg\\node-modules\\pnpm-demo\\node_modules\\.pnpm\\loose-envify@1.4.0\\node_modules\\js-tokens"
},
{
"name": "loose-envify",
"version": "1.4.0",
"dir": "~\\app-builder\\pkg\\node-modules\\pnpm-demo\\node_modules\\.pnpm\\react@18.2.0\\node_modules\\loose-envify"
},
{
"name": "react",
"version": "18.2.0",
"dir": "~\\app-builder\\pkg\\node-modules\\pnpm-demo\\node_modules\\react"
}
] In the new format, it's clearer to see which packages are depended on and which ones are conflicting. |
I have already integrated the new implementation method in this PR(beyondkmp#2) and passed all the tests. When you have time, please review it. If there are no issues, I can first submit a PR to app-builder and then submit this PR to electron-builder. |
Here's app-builder PR(develar/app-builder#116) |
The app-builder PR(develar/app-builder#116) is merged. Please help publish a alpha build(v5.0.0-alpha.3) for app-builder. |
closed as #8190. |
No description provided.